Repository navigation
fix(cluster): refuse an unroutable command before recording anything - #133
Merged
Merged
Conversation
A command the cluster connection cannot route (keys spanning several slots where one is required, or a command with the `special` request policy) was refused only when the network task fed it, after the handler had already recorded its bookkeeping and fed the commands ahead of it: - a refused SSUBSCRIBE left its pending batch at the head of the router's subscription queue; the next subscription's confirmation was matched against it and its caller was never answered; - in a pipeline, the commands fed ahead of the refused one were written and their replies were handed to the next callers; - `send_messages` returned on the error, stranding every message behind it in the send queue until something else woke the send path, and leaking the refused message's byte charge from the send-queue budget. The routing decision now lives in one classifier, `route`, read both by the feed path and by `check_routable`, which the handler calls before anything is recorded or queued. A feed that still fails (a broken node connection, a slot no node covers) fails its own message, releases its charge, and lets the rest of the batch be fed and flushed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jo8jBqM8KTsnpJz9oRkK5G
`a_read_goes_to_a_replica_when_the_read_preference_asks_for_it` polls the replica until the value written to the master has reached it, but read it as a `String`. Until replication lands, the replica answers nil, which a `String` refuses with `UnexpectedNil`, and the `?` ended the test on the first attempt: the retry loop never ran. Seen on CI as `Received response Null` followed by the failure. The value is now read as an `Option<String>`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jo8jBqM8KTsnpJz9oRkK5G
Member
Author
|
CI on Fixed in b2d29ec by reading an Generated by Claude Code |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A command the cluster connection cannot route (keys spanning several slots where one is required, or a command with the
specialrequest policy) was refused only when the network task fed it, after the handler had already recorded its bookkeeping and fed the commands ahead of it. Three consequences, each reproduced by a new test that fails onmain:SSUBSCRIBEleft its pending batch at the head of the router'spending_subscriptions. Only the head is ever matched, so the confirmation of the nextssubscribewas consumed by the ghost batch (still waiting for its other channel) and that caller was never answered. This is very likely the "never gets a message" symptom of the original report.GETreceived theOKof an orphanedSET).send_messagesreturned on the first feed error: every message behind it in the same batch stayed in the send queue until another message woke the send path (a caller on an idle client waited forever), and the failed message's byte charge was never released from the send-queue budget.Fix
route(command) -> Result<Route>, read both byinternal_feedand byClusterConnection::check_routable. What the handler accepts up front is therefore exactly what the feed serves; nothing is duplicated.NetworkHandler::handle_messagecallsConnection::check_routableon every command of a message first, before any pub/sub bookkeeping, shedding decision or queuing. A refused message leaves no trace and writes nothing. Standalone and sentinel connections accept everything, as before.Side effect: the
CommandNotSupportedInClustererror now carries the command name in its context, likeMismatchedKeySlotsalready did.Known limitation
A residual feed error in the middle of a multi-command message (I/O or topology, no longer a routing refusal) can still leave earlier commands of that message written. Those errors come with a broken or reconfigured connection, which the reconnection path handles; making that case exact would need per-command rollback in the cluster request queue, which is out of scope here.
Tests
a_refused_cross_slot_ssubscribe_leaves_the_client_usable(pub/sub): times out onmain, passes here.a_pipeline_with_a_cross_slot_command_runs_nothing(cluster): desyncs onmain, passes here.a_feed_error_does_not_strand_the_rest_of_the_batch(cluster): uses a new one-shotClusterTestHook::arm_feed_error_forto fail a feed past the routing check; times out onmain, passes here.cargo clippy --all-targets -- -D warnings(both CI feature sets),./run_tests.sh --hermetic, and the pub/sub and cluster suites on a local 3-master cluster: no new failure.Generated by Claude Code